-
Notifications
You must be signed in to change notification settings - Fork 35
CDN - Add Geofencing #994
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
CDN - Add Geofencing #994
Conversation
@matheuspolitano Could you please add a description to the PR which describes what you want to implement here? |
And I would kindly ask you to squash your commits into some meaningful ones. Having these 26 commits makes not much sense to me. |
On hold until #983 is merged. |
@matheuspolitano Could you please resolve the conflicts? :) |
df477c5
to
af17e1c
Compare
Co-authored-by: Marcel Jacek <[email protected]>
Co-authored-by: Marcel Jacek <[email protected]>
Co-authored-by: Marcel Jacek <[email protected]>
Co-authored-by: Marcel Jacek <[email protected]>
// convertPointerSliceToStringSlice safely converts a slice of string pointers to a slice of strings. | ||
func convertPointerSliceToStringSlice(pointerSlice []*string) []string { | ||
if pointerSlice == nil { | ||
return nil // Or []string{} depending on how you want to handle it | ||
} | ||
stringSlice := make([]string, 0, len(pointerSlice)) | ||
for _, strPtr := range pointerSlice { | ||
if strPtr != nil { // Safely skip any nil pointers in the list | ||
stringSlice = append(stringSlice, *strPtr) | ||
} | ||
} | ||
return stringSlice | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this to the utils package and add a few tests for this function? This is a function we probably need in some other places as well.
oldCountriesPtrs := oldGeofencingMap[url] | ||
|
||
oldCountries := convertPointerSliceToStringSlice(oldCountriesPtrs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it could be the case, that the old map might doesn't have the url, e.g. if someone extended the geofencing values via the api directly. So you need to check here, if the oldMap has the url key and if not, it must be handled correctly
geofencingCountry := make([]string, len(countryCodes)) | ||
for i, countryCodePtr := range countryCodes { | ||
if countryCodePtr == nil { | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an url has only one country code and this one is set to null, it will result in an api error, because the URL would have an empty array. So I would return here an error instead of continue
countryStrings := make([]string, len(countries)) | ||
for i, countryPtr := range countries { | ||
if countryPtr != nil { | ||
countryStrings[i] = *countryPtr | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should return here an error, if coutryPtr is nil to prevent api issues. This could happen if someone sets null within the country list and then an empty array will be send to the api
Description
https://jira.schwarz/browse/STACKITCDN-875
Checklist
make fmt
examples/
directory)make generate-docs
(will be checked by CI)make test
(will be checked by CI)make lint
(will be checked by CI)